Add message, stream and URI factories for Slim Framework #53

tuupola · 2016-10-13T20:21:25Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets
Documentation	php-http/documentation#159
License	MIT

What's in this PR?

This PR adds message, stream and URI factories for Slim Framework

Why?

Slim is reasonably popular PSR-7 based framework.

Example Usage

use Http\Client\Curl\Client;
use Http\Message\MessageFactory\SlimMessageFactory;
use Http\Message\StreamFactory\SlimStreamFactory;

$client = new Client(new SlimMessageFactory(), new SlimStreamFactory());

Checklist

Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
Documentation pull request created (if not simply a bugfix)

To Do

Should something be added to puli.json?
Should these be named Slim3StreamFactory and Slim3StreamFactory instead?
Should there be more tests? This was so easy I feel like I am missing something.

tuupola · 2016-10-13T20:25:27Z

Build fails on PHP 5.4 because Slim requires PHP >=5.5.0.

sagikazarmark · 2016-10-13T23:04:51Z

@tuupola thank you for the PR. Can you try adding https://github.com/akeneo/PhpSpecSkipExampleExtension and skip the tests on PHP 5.4?

tuupola · 2016-10-14T07:24:00Z

That might not be enough since also composer install fails. I see what I can do with the Travis build matrix.

sagikazarmark · 2016-10-14T08:18:50Z

@php-http/owners how about dropping PHP 5.4 support of this package too?

Nyholm · 2016-10-14T08:29:41Z

Dropping 5.4 is a no-brainer in my opinion. My general approach is that you should keep old PHP versions if there is no reson for bumping then. I believe @tuupola has an excellent reson here.

We had a discussion on twitter about supporting 5.4 because legacy project would not be able to update to HTTPlug without dropping support for 5.4. (Because of php-http/discovery).

xabbuh · 2016-10-14T09:30:01Z

As this library is only a dependency during development I would not use it as an argument to bump the minimum version requirements. Instead I would remove the dev requirement and install the package on Travis CI only for PHP >= 5.5 jobs and skip the example otherwise as @sagikazarmark suggested.

sagikazarmark · 2016-10-14T09:33:45Z

@xabbuh the problem is that this way development is much harder. I would do the other way around: remove the library in CI on PHP 5.4. That way you can't develop it if you have 5.4, but the lib remains compatible with it.

Nyholm · 2016-10-14T09:34:29Z

Sure, Im fine with that.

xabbuh · 2016-10-14T09:35:29Z

@sagikazarmark Sounds good too.

tuupola · 2016-10-14T09:42:58Z

I already have it the way @xabbuh explained. However comment by @sagikazarmark makes sense. I will do it over and commit soonish.

sagikazarmark · 2016-10-14T10:20:36Z

.travis.yml

@@ -35,6 +35,9 @@ before_install:
 install:
    - travis_retry composer update ${COMPOSER_FLAGS} --prefer-dist --no-interaction

+before_script:
+    - if [[ "${TRAVIS_PHP_VERSION}" == "5.4" ]]; then composer remove slim/slim; fi


I would move this to before install AND add --no-update flag to composer. That way we only have to run compose once.

sagikazarmark · 2016-10-14T10:21:18Z

composer.json

@@ -14,19 +14,22 @@
        "php": ">=5.4",
        "psr/http-message": "^1.0",
        "php-http/message-factory": "^1.0.2",
-        "clue/stream-filter": "^1.3"
+        "clue/stream-filter": "^1.3",
+        "slim/slim": "^3.5"


This is only dev requirement, right?

Should be yes. But this way it still wont still work because. You cannot do the composer install and then composer remove slim/slim with PHP 5.4 anyway since it is the composer install which will fail. I try if --ignore-platform-reqs helps.

Not sure I understand you. Also, please see my previous comment about moving the removal to before_install instead of before_script with --no-update. --no-update causes to update composer.json, but does not actually call install.

Ah disregard last comment. I did not know you can run composer remove with empty vendor.

sagikazarmark · 2016-10-14T11:03:44Z

composer.json

    },
    "suggest": {
        "zendframework/zend-diactoros": "Used with Diactoros Factories",
        "guzzlehttp/psr7": "Used with Guzzle PSR-7 Factories",
+        "slim/slim": "Used with Slim Factories",


Diactoros and Guzzle PSR-7 are obviously meaning message factories while Slim is a framework. Can you please add some extra indication that it means message/PSR-7 factories? Just to avoid confusion.

Not sure about wording. Maybe "Used with Slim Framework PSR-7 implementation" or "Used with Slim Framework PSR-7 messages"?

Any of those work for me

tuupola · 2016-10-14T11:09:21Z

Seems there is problem with streams with hhvm. Will install hhvm locally to figure it out.

facebook/hhvm#6999

tuupola · 2016-10-14T12:26:43Z

I would also like to add something to message factory tests. Namely I would like to test that factories set the message headers and body correctly. Should this be a separate PR?

Nyholm · 2016-10-14T21:28:26Z

Great job. Thank you!

sagikazarmark · 2016-10-14T22:22:36Z

@Nyholm there were some TODOs which indeed would have been nice to have here.

Nyholm · 2016-10-14T22:24:42Z

Sorry, I must have missed that. I saw that you approved it so I made a final review myself.

What were the TODOs?

sagikazarmark · 2016-10-14T22:25:45Z

See the TODO section: puli.json, possibly rename to Slim3

Nyholm · 2016-10-14T22:29:02Z

Sorry for not bringing those up to discussion before merging.

Yes, some entries should be added to Puli.json. The other two are fine IMHO

tuupola · 2016-10-15T06:41:07Z

I am not familiar with Puli. How should I add the entries to puli.json? I quickly read through puli docs but did not become any smarter.

Add message, stream and URI factories for Slim Framework

021f8e3

tuupola added 2 commits October 14, 2016 13:17

Do not test Slim factories with PHP 5.4

9e13119

Add missing test for Slim stream factory

f5302d5

sagikazarmark requested changes Oct 14, 2016

View reviewed changes

tuupola added 2 commits October 14, 2016 14:00

Slim should be development dependency

fb74a28

Remove slim/slim before running composer install on PHP 5.4

bdce0e2

sagikazarmark requested changes Oct 14, 2016

View reviewed changes

tuupola added 2 commits October 14, 2016 15:10

HHVM does not have rw mode, use r+ instead

ea80b63

facebook/hhvm#6999

Better wording for suggestion

869d658

tuupola mentioned this pull request Oct 14, 2016

Mention Slim Framework factories php-http/documentation#159

Merged

Mention Slim Framework factories

d41f5e7

tuupola mentioned this pull request Oct 14, 2016

Test also body and headers in message factory tests #54

Merged

sagikazarmark approved these changes Oct 14, 2016

View reviewed changes

Nyholm merged commit 51a0aa5 into php-http:master Oct 14, 2016

This was referenced Oct 15, 2016

Relax minimum required version of Slim #57

Merged

Discovery support for Slim Framework factories php-http/discovery#80

Merged

Add message, stream and URI factories for Slim Framework #53

Add message, stream and URI factories for Slim Framework #53

Uh oh!

Conversation

tuupola commented Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this PR?

Why?

Example Usage

Checklist

To Do

Uh oh!

tuupola commented Oct 13, 2016

Uh oh!

sagikazarmark commented Oct 13, 2016

Uh oh!

tuupola commented Oct 14, 2016

Uh oh!

sagikazarmark commented Oct 14, 2016

Uh oh!

Nyholm commented Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xabbuh commented Oct 14, 2016

Uh oh!

sagikazarmark commented Oct 14, 2016

Uh oh!

Nyholm commented Oct 14, 2016

Uh oh!

xabbuh commented Oct 14, 2016

Uh oh!

tuupola commented Oct 14, 2016

Uh oh!

sagikazarmark Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

sagikazarmark Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

tuupola Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

tuupola Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

sagikazarmark Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

tuupola Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

sagikazarmark Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

tuupola commented Oct 14, 2016

Uh oh!

tuupola commented Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nyholm commented Oct 14, 2016

Uh oh!

sagikazarmark commented Oct 14, 2016

Uh oh!

Nyholm commented Oct 14, 2016

Uh oh!

sagikazarmark commented Oct 14, 2016

Uh oh!

Nyholm commented Oct 14, 2016

Uh oh!

tuupola commented Oct 15, 2016

Uh oh!

Uh oh!

tuupola commented Oct 13, 2016 •

edited

Loading

Nyholm commented Oct 14, 2016 •

edited

Loading

tuupola Oct 14, 2016 •

edited

Loading

tuupola commented Oct 14, 2016 •

edited

Loading